Skip to content

Conversation

@Anushreebasics
Copy link

issue #13328

Summary
This PR fixes the SSD serialization logic so that objects saved with SSD.save() can be fully restored using SSD.read() without losing internal state.
Previously, some fitted attributes required to reconstruct a trained SSD instance were not persisted, which caused test_ssd_save_load to fail during deserialization.

What was changed?
Refactored the SSD.save() method to persist all fitted attributes required to restore a trained instance.
Updated the corresponding SSD.read() logic to correctly re-initialize these attributes when loading from disk.
Ensured that save/load performs a true round-trip and yields an SSD instance equivalent to the original fitted object.

Copilot AI review requested due to automatic review settings January 13, 2026 16:03
@Anushreebasics
Copy link
Author

Anushreebasics commented Jan 13, 2026

@larsoner @mscheltienne Kindly review this PR and let me know if any changes are required

This comment was marked as off-topic.

@tsbinns
Copy link
Contributor

tsbinns commented Jan 13, 2026

I'm happy to look over this @larsoner

@tsbinns
Copy link
Contributor

tsbinns commented Jan 13, 2026

@Anushreebasics Thanks for opening the PR.

It would be good to set some explicit __get_state__ and __set_state__ methods as per this comment, much like we do for Spectrum and TFR objects (e.g., for TFR objects).

Additionally, we should really also add this save/load behaviour to SPoC, like suggested in the issue/forum post. Actually, the format of generalised eigenvalue decomposition tools was standardised with introduction of the _GEDTransformer class, so now it might make sense to also add the features to the CSP and XdawnTransformer classes (especially now that SPoC inherits from CSP). Though perhaps there might be some caveats making this more complex than I imagined.

Please have a think about adding __get_state__ and __set_state__ methods for the _GEDTransformer class that can be customised for the individual GED classes. For an example, you can have a look at the AverageTFR and EpochsTFR classes to see how their __get_state__ and __set_state__ methods build on those from BaseTFR.

@Anushreebasics
Copy link
Author

Anushreebasics commented Jan 14, 2026

@tsbinns please review the changes
I’ve added explicit getstate / setstate methods to SSD, building on those in _GEDTransformer, following the same pattern used by BaseTFR. Serialization is now explicit, avoids storing callables, and reconstructs the covariance estimator deterministically on load.
For this PR, I am intentionally limiting the scope to SSD only, to ensure the proposed getstate / setstate approach is correct and aligned with MNE’s design expectations. If this implementation looks good, I will follow up with a separate PR extending the same pattern to SPoC.

Copy link
Contributor

@tsbinns tsbinns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes @Anushreebasics. A couple comments and suggestions here.

Also, make sure you add any new functions to the __init__.pyi file, otherwise they can't be imported. This also means that the test which has been added can't run at the moment. Please have a go at running any tests you are adding/changing locally first, that way you'll have a much clearer idea of how the changes are (mis)behaving.

You can find info about running the tests in the contribution guide (here and here). Feel free to ask if you have questions about getting this running.

@Anushreebasics
Copy link
Author

@tsbinns please review the changes

This comment was marked as spam.

@mne-tools mne-tools deleted a comment from Copilot AI Jan 18, 2026
@mne-tools mne-tools deleted a comment from Copilot AI Jan 18, 2026
@mne-tools mne-tools deleted a comment from Copilot AI Jan 18, 2026
@mne-tools mne-tools deleted a comment from Copilot AI Jan 18, 2026
@mne-tools mne-tools deleted a comment from Copilot AI Jan 18, 2026
@mne-tools mne-tools deleted a comment from Copilot AI Jan 18, 2026
@tsbinns
Copy link
Contributor

tsbinns commented Jan 18, 2026

@Anushreebasics I can't see any changes apart from merging the main branch. Please check that you have pushed the commits with changes.

@Anushreebasics
Copy link
Author

@tsbinns sorry it was not pushed, please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants